-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[톰캣 구현하기 - 2, 3, 4단계] 밀리(김미성) 미션 제출합니다. #464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 밀리~
미션 진행하시느라 고생하셨습니다.
지난번에 리뷰드렸던 양방향 의존성 문제 깔끔하게 해결 잘 해 주셨어요! 👍
코드를 너무 잘 짜주셔서 딱히 리뷰할 부분을 찾느라 고생했네요.. 🥲
커멘트 남긴 거 확인해주시면 감사하겠습니다~
너무 고생하셨어요! 👍👍
final Session session = new Session(); | ||
sessionManager.add(session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 상태도 좋지만 세션 생성의 책임을 sessionManager에게 위임하는건 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇게 하니 책임 분리가 더 잘 된 느낌이네요!
저는 처음 구현할 때 저희가 개발하는 어플리케이션을 생각해보면 세션이나 jwt를 직접 생성해서 데이터베이스에 저장하던지, 메모리로 갖고 있던지 하기 때문에 로그인 로직에서 생성하도록 했었어요!
계속 고민을 해봤는데 SessionManager의 역할이 조금 헷갈려서 그러는데 혹시 말랑은 SessionManager에서 생성하는 책임을 갖도록 하신 이유가 무엇인가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SessionManager라는 이름부터, 세션의 전체 생명주기를 관리하는 것 같다는 느낌이 들었어요!
tomcat/src/main/java/org/apache/coyote/handler/FrontController.java
Outdated
Show resolved
Hide resolved
@@ -4,5 +4,5 @@ | |||
|
|||
public interface BodyParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해 커멘트에 답글 남겨드렸는데요, 현재 상태에서 Body가 json인지, form data인지 구분해서 처리할 수 있는 구조인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 HttpRequestParser
클래스 생성자에서
bodyParsers.put("application/x-www-form-urlencoded", new FormBodyParser());
이와 같은 작업을 해서 map으로 관리하고 있습니다!
추후에 JsonBodyParser
가 구현이 된다면 map에 추가를 해주는 방식을 생각했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 좋습니다 ~ 👍👍👍
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/HttpRequestParser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 말랑!
정말 빠르게 리뷰 주셨는데 요즘 너무 바빠서 결국 또 늦어졌네요🥲
리뷰 남겨주신 것 반영하고 코멘트도 남겼습니다!
확인 부탁드려요. 감사합니다!!
@@ -4,5 +4,5 @@ | |||
|
|||
public interface BodyParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 HttpRequestParser
클래스 생성자에서
bodyParsers.put("application/x-www-form-urlencoded", new FormBodyParser());
이와 같은 작업을 해서 map으로 관리하고 있습니다!
추후에 JsonBodyParser
가 구현이 된다면 map에 추가를 해주는 방식을 생각했습니다!
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
final Session session = new Session(); | ||
sessionManager.add(session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇게 하니 책임 분리가 더 잘 된 느낌이네요!
저는 처음 구현할 때 저희가 개발하는 어플리케이션을 생각해보면 세션이나 jwt를 직접 생성해서 데이터베이스에 저장하던지, 메모리로 갖고 있던지 하기 때문에 로그인 로직에서 생성하도록 했었어요!
계속 고민을 해봤는데 SessionManager의 역할이 조금 헷갈려서 그러는데 혹시 말랑은 SessionManager에서 생성하는 책임을 갖도록 하신 이유가 무엇인가요??
tomcat/src/main/java/org/apache/coyote/handler/FrontController.java
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! 0 Bugs 0.0% Coverage The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어요 밀리~
이전보다 가독성도 좋아졌고, 각 객체들이 적절한 책임을 가지고 있도록 바뀐 것 같아요!
코드를 너무 잘 작성해 주셔서 더이상 개선할 부분이 눈에 보이지 않네요~
(딱 하나 생각해 볼 만한 간단한 커멘트 남겼으니 확인 부탁드려요!)
미션은 머지하겠습니다~ 미션 고생하셨어요!
@@ -12,6 +12,7 @@ public class IndexController extends AbstractController { | |||
public void doGet(final HttpRequest request, final HttpResponse response) { | |||
final String responseBody = ViewResolver.read("/index.html"); | |||
response.setResponseBody(responseBody); | |||
response.setContentLength(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한가지만 더 질문을 드리고 싶은데요,
ContentLength는 ResponseBody가 정해짐에 따라 항상 함께 정해져야 하는 것 아닐까요?
즉 이렇게 setResponseBody 이후에 setContentLength를 해주는 게 아니라 setResponseBody 내부에서 setContentLength를 처리해 주는 것은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오!! 저도 이 부분에 대해서 고민을 했었어요!
항상 함께 정해져야 하는게 맞는 것 같다가도, responseBody를 set하는데 Content-Length가 지정될 지 예측을 할 수 있을까? 하는 생각에 그냥 분리했습니다! 당연히 http에 대해서 잘 안다면 content-length가 지정될 수 있겠구나 생각은 할 수 있겠지만요.
내부 코드를 보지 않으면 content-length가 두 번 지정될 수도 있을 것도 같네요.
그런데 해당 메서드를 사용하는 사용자 입장에서 무엇이 더 편할지는 또 고민이 되네요..
말랑은 어떻게 생각하시나요?!
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
import org.apache.coyote.http11.HttpStatusCode; | ||
import org.apache.coyote.http11.ViewResolver; | ||
|
||
public class StaticController extends AbstractController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 것 같아요👍👍
final Session session = new Session(); | ||
sessionManager.add(session); | ||
session.setAttribute(session.getId(), findUser.get()); | ||
final Session session = sessionManager.create(findUser.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다 👍👍👍
안녕하세요 말랑!
머지 이후로 시간이 많이 지났네요..!
이전 단계에서 남겨주신 코멘트 덕분에 양방향 의존성에 대해서 정말 많이 고민했었는데요.
덕분에 리팩토링하면서 구조가 깔끔해지는 것을 느껴서 너무 재밌었어요😊
감사합니다 ㅎㅎ
이번에도 잘 부탁드립니다🙇♀️